Skip to content

Conversation

kernel-patches-daemon-bpf[bot]
Copy link

Pull request for series with
subject: bpf: Fix possible memleak when updating hash and local storage maps
version: 2
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013677

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 1c64efc
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013677
version: 2

@kernel-patches-review-bot
Copy link

commit 720324fbde9db01809d09f823db683b6b6729314
Author: Leon Hwang <[email protected]>

bpf: Fix possible memleak when updating local storage maps with BPF_F_LOCK

This patch fixes a memory leak when updating local storage maps with
BPF_F_LOCK by calling bpf_obj_free_fields() after copy_map_value_locked()
to properly release old special fields like BPF_KPTR_{REF,PERCPU}.

> diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c
> index b931fbceb..2b7bd47e9 100644
> --- a/kernel/bpf/bpf_local_storage.c
> +++ b/kernel/bpf/bpf_local_storage.c
> @@ -609,6 +609,7 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
>  		if (old_sdata && selem_linked_to_storage_lockless(SELEM(old_sdata))) {
>  			copy_map_value_locked(&smap->map, old_sdata->data,
>  					      value, false);
> +			bpf_obj_free_fields(smap->map.record, old_sdata->data);
>  			return old_sdata;
>  		}
>  	}

Does the same issue exist in the locked path later in this function?

In bpf_local_storage_update(), there's a second location where
copy_map_value_locked() is called with BPF_F_LOCK (around line 642-646
in the patched version). This path also updates old_sdata->data in place:

	if (old_sdata && (map_flags & BPF_F_LOCK)) {
		copy_map_value_locked(&smap->map, old_sdata->data, value,
				      false);
		selem = SELEM(old_sdata);
		goto unlock;
	}

This appears to be the same pattern as the fixed code above - it calls
copy_map_value_locked() to update in place, but doesn't call
bpf_obj_free_fields() to free the old special fields. Can the old kptr
fields leak here too?

Both paths handle in-place updates with BPF_F_LOCK:
- Line 609-613 (lockless fast path): copy + free (fixed by this patch)
- Line 642-646 (locked slow path): copy only (missing free?)

The hashtab code in kernel/bpf/hashtab.c has a similar pattern at lines
1124-1127 and 1153-1156 where copy_map_value_locked() is followed by
check_and_free_fields(), which calls bpf_obj_free_fields().



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf: Fix possible memleak when updating local storage maps with BPF_F_LOCK
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18659108882

@kernel-patches-daemon-bpf
Copy link
Author

Forwarding comment 3423008850 via email
In-Reply-To: [email protected]
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

As [lru_,]percpu_hash maps support BPF_KPTR_{REF,PERCPU}, missing
calls to 'bpf_obj_free_fields()' in 'pcpu_copy_value()' can leak memory
referenced by BPF_KPTR_{REF,PERCPU} fields.

Fix this by calling 'bpf_obj_free_fields()' after
'copy_map_value[,_long]()' in 'pcpu_copy_value()'.

Fixes: 65334e6 ("bpf: Support kptrs in percpu hashmap and percpu LRU hashmap")
Signed-off-by: Leon Hwang <[email protected]>
When updating hash maps with BPF_F_LOCK, the special fields were not
freed after being replaced. This could cause memory referenced by
BPF_KPTR_{REF,PERCPU} fields to leak.

Fix this by calling 'check_and_free_fields()' after
'copy_map_value_locked()' to properly release the old fields.

Fixes: 14a324f ("bpf: Wire up freeing of referenced kptr")
Signed-off-by: Leon Hwang <[email protected]>
…_LOCK

When updating local storage maps with BPF_F_LOCK, the special fields
were not freed after being replaced. This could cause memory referenced
by BPF_KPTR_{REF,PERCPU} fields to leak.

Fix this by calling 'bpf_obj_free_fields()' after
'copy_map_value_locked()' to properly release the old fields.

Fixes: 9db44fd ("bpf: Support kptrs in local storage maps")
Signed-off-by: Leon Hwang <[email protected]>
…cgrp storage maps

Add tests to verify that updating hash and local storage maps does not
leak memory when BPF_KPTR_REF objects are involved.

The tests perform the following steps:

1. Call update_elem() to insert an initial value.
2. Use bpf_refcount_acquire() to increment the refcount.
3. Store the node pointer in the map value.
4. Add the node to a linked list.
5. Probe-read the refcount and verify it is *2*.
6. Call update_elem() again to trigger refcount decrement.
7. Probe-read the refcount and verify it is *1*.

Signed-off-by: Leon Hwang <[email protected]>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 1c64efc
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1013677
version: 2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant